Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Calls Webhook on Cohort Update #18735

Closed
wants to merge 8 commits into from

Conversation

ahsan722505
Copy link
Contributor

Problem

#18083

Changes

image

How did you test this code?

I tested it manually by shortening the duration of cohort calculation and adding the people to cohort. Then I monitored the API endpoint that was being called when the cohort got updated.

@ahsan722505 ahsan722505 changed the title Calls Webhook on Cohort Update feat: Calls Webhook on Cohort Update Nov 18, 2023
@ahsan722505
Copy link
Contributor Author

Hey! Some tests are failing in this PR as I introduced a field "post_to_webhook" to "cohort" table. I tried fixing the failing tests by rerunning them with "--snapshot-update" argument, but still some tests are failing. Can anyone help out here?

@neilkakkar
Copy link
Contributor

Hey @ahsan722505 , thanks for the contribution! I'm not sure if this addresses the use case in the issue. They need the people who've moved in / out of the cohort, and use that to update following systems.

It becomes much less useful imo just to know that the cohort has been changed. We're also currently revamping the webhook system to make it much more reliable / handle retries / give analytics into failed/successful hooks, and it would be nice to integrate into that system vs calling endpoints from the middle of our code.

So, I'd say wait for the above to happen before tackling this change.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@posthog-bot posthog-bot closed this Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants